fix(security): validate account names to prevent Numscript injection (EN-1200)#106
fix(security): validate account names to prevent Numscript injection (EN-1200)#106flemzord wants to merge 8 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThe PR tightens ChangesIdentifier Injection Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
flemzord
changed the title
flemzord
left a comment
flemzord
left a comment
There was a problem hiding this comment.
Revue inline: j’ai relevé deux trous de validation restants dans le durcissement des segments de compte.
🛑 Changes requested — multi-model reviewThe PR correctly anchors the balance name regex and adds injection validation for wallet subjects and wallet IDs, closing the primary Numscript injection vectors. However, two major issues require attention before merging. First, removing '-' from valid balance names goes beyond the stated anchoring fix: the dash character is not itself a Numscript injection vector, and the change silently breaks debit operations for any pre-existing wallet whose balances have dashed names (since names are re-validated against the new regex when read back from the ledger). This is a backwards-incompatible behavioral change that needs either a rollback to permitting dashes or a migration/compatibility plan. Second, WalletID validation for the Credit path is performed inside the manager method rather than inside Credit.Validate(), creating an inconsistency with the Debit path and a potential bypass for non-manager callers. Additionally, there are no handler-level tests exercising malicious walletIDs supplied via URL parameters, and the stated rationale for dash exclusion (Address.String() aliasing) lacks a confirming test or code reference. 🟠 [major] Removing '-' from balance names breaks backwards compatibility with existing data
The regex change from Suggestion: Either keep '-' in the allowed character set while still anchoring the regex (i.e. 🟠 [major] WalletID validation for Credit lives outside Validate(), making it bypassable
The WalletID validation in the Credit flow is performed directly inside Manager.Credit via accountSegmentRegexp rather than inside Credit.Validate(). This means any other caller of Credit.Validate() (e.g. tests or future handlers) will not have the WalletID checked. This is an inconsistency with how Debit.Validate() handles its WalletID, and represents a potential bypass vector if the manager method is not the sole entry point in the future. Suggestion: Move the 🟡 [minor] No handler-level tests for malicious URL wallet IDs
The new tests cover balance names and WALLET subject identifiers, but there are no handler tests that exercise a malicious walletID coming in via the URL path (e.g. Suggestion: Add credit and debit handler tests using URL walletIDs containing account separators or Numscript tokens (e.g. 🟡 [minor] Dash-exclusion rationale for balance names is unverified and should have a confirming test
Subject.Validate() enforces stricter balance-name rules (no '-') compared to wallet identifiers, with the justification being that Address.String() strips dashes and causes aliasing. However, there is no test or code reference that actually demonstrates this aliasing behavior. Without such evidence, the stricter rule appears arbitrary and the security rationale is unverifiable. Suggestion: Add a test demonstrating that Chart().GetBalanceAccount with a dashed name aliases to the undashed account, or add a code comment citing the specific ledger Address.String() behavior that causes the aliasing concern. ⚪ [nit] Mock transaction creator returns nil,nil which could mask nil-deref panics
In TestWalletsDebitRejectsInvalidBalanceMetadata, the WithCreateTransaction mock returns (nil, nil). If the validation check is ever accidentally removed and the code proceeds to use the nil transaction result, the test would panic rather than produce a clean assertion failure, potentially masking regressions. Suggestion: Return a minimal non-nil Reviewed in parallel by claude (anthropic/claude-opus-4-8) and gpt (openai/gpt-5.5), then consolidated. This comment is updated on each push. |
The credit/debit Numscript templates interpolate account names directly
into the script body (@{{ .source }}). WALLET subjects only checked that
the identifier was non-empty, and the balance-name regex was unanchored
(MatchString matches any substring), so a crafted identifier/balance/name
containing newlines or Numscript tokens could break out of the source
block and inject arbitrary statements executed with the service ledger
credentials.
- Anchor balanceNameRegex (^...$) so a name must be a single clean segment
- Validate WALLET subject identifier and balance via accounts.ValidateAddress
- Validate walletID before building debit/credit scripts
Adds regression tests for separator/whitespace/token names and for
injection via a WALLET source identifier and balance.
…nces Address review feedback: accounts.ValidateAddress accepts a full multi-segment address, so a WALLET subject with identifier/balance like "foo:bar" passed and resolved to a nested account outside the balance model. Validate wallet IDs and balance names against an anchored single-segment regex instead, keeping ValidateAddress only for ACCOUNT subjects. Documents the residual dash-stripping collision in Address.String() (kept as a separate, migration-bearing change rather than forbidding dashes, which would break legitimate dashed/UUID balance names).
Address.String() strips all '-' after joining segments, so inputs
differing only by dashes collapse to the same ledger account
("foo-bar" == "foobar"). This affects wallet IDs, balance names and
hold IDs alike and can silently make two distinct entities share one
account on create/get/debit/credit.
Constraint: the strip cannot be removed without changing the address of
every already-created wallet/balance/hold (needs a data migration)
Directive: do not remove the strip or relax segment validation without a
migration plan — the proper fix (stop stripping + migrate) is a separate ticket
Confidence: high
Scope-risk: narrow
The WalletID was validated ad-hoc inside Manager.Credit, so any caller invoking Credit.Validate() directly (tests, future handlers) skipped the check. Move it into Credit.Validate() — mirroring Debit.Validate() — so the single-segment guard cannot be bypassed, and drop the duplicate check from the manager. Constraint: WalletID is interpolated as a chart segment, so it must be a single anchored segment with no ':' or Numscript metacharacters Rejected: keep the check only in the manager | bypassable by non-manager callers Confidence: high Scope-risk: narrow
Add credit and debit handler tests that send a WalletID via the URL path
spanning multiple account segments ("wallet:injected") or carrying
Numscript tokens ("wallet\n@world"), asserting a 400 and that no ledger
transaction is created. Also return a non-nil transaction from the
WithCreateTransaction mock in TestWalletsDebitRejectsInvalidBalanceMetadata
so a regression surfaces as an assertion failure rather than a nil-deref panic.
Confidence: high
Scope-risk: narrow
Re-allow '-' in balance names (^[0-9A-Za-z_-]+$), matching the charset already used for wallet IDs. The anchoring remains the real injection fix (no ':' separator, whitespace or Numscript tokens); dashes are not an injection vector, and forbidding them regressed legitimate dashed/UUID balance names — notably read-back validation in Manager.Debit would reject pre-existing balances. Dashes still alias under Address.String() (it strips '-', so "foo-bar" and "foobar" collapse to one ledger account). This affects every Chart-routed segment — wallet IDs, balance names, hold IDs — but not SubjectTypeLedgerAccount identifiers, which bypass the strip. The comments on balance.go/subject.go/chart.go now document that the collision is live, not theoretical. Constraint: dashes must stay valid — wallet/hold IDs are UUIDs and dashed balance names already exist in production data Rejected: forbid '-' to dodge aliasing | regresses existing data; aliasing also affects wallet/hold IDs, so a regex tweak is not a real fix Directive: the aliasing root cause (Address.String stripping '-') needs a separate ticket (stop stripping + migrate) — do not "fix" it via validation Confidence: high Scope-risk: moderate
Flip the dash cases to success now that '-' is allowed: balance creation,
a dashed balance in a credit wallet source, a dashed credit destination,
and a dashed debit balance source all resolve and post a transaction.
TestWalletsDebitRejectsInvalidBalanceMetadata now stores a genuine
injection name ("injected\n@world") so it still proves read-back
validation rejects Numscript tokens rather than a now-valid dash.
Confidence: high
Scope-risk: narrow
fguery
force-pushed
the
fix/numscript-injection-validation
branch
from
8800664 to
9c0eb06
Compare
NumaryBot
left a comment
NumaryBot
left a comment
There was a problem hiding this comment.
🛑 Changes requested — automated review
The new validation can reject the service's own UUID wallet IDs before chart normalization, breaking core credit/debit flows for normal wallets.
| // individual chart segments, so — unlike a full ledger address — they must | ||
| // not contain ':' (which would resolve to a nested account outside the | ||
| // expected balance model and is the Numscript-injection vector). | ||
| var accountSegmentRegexp = regexp.MustCompile("^" + accounts.SegmentRegex + "$") |
There was a problem hiding this comment.
🔴 [blocker] Allow generated UUID wallet IDs through validation
When credit/debit is called for a normal wallet created by this service, the wallet ID is a UUID containing -, but this regexp validates the raw ID as a ledger segment before Chart.String() gets a chance to normalize it by stripping dashes. Since ledger segments do not admit dashes, these new checks reject existing/generated wallet IDs and make credit/debit (and WALLET subjects using real wallet IDs) fail validation.
There was a problem hiding this comment.
Not a blocker — accounts.SegmentRegex is [a-zA-Z0-9_-]+, so dashes are valid in a ledger segment and a UUID wallet ID passes accountSegmentRegexp without needing Address.String() to normalize it. Added TestWalletIDValidationAcceptsGeneratedUUID (b89d553) to prove generated UUIDs pass credit/debit/subject validation while :-bearing IDs are still rejected.
|
@flemzord I allowed back the "-" in the various places; if we don't and we're using uuid we kinda have a problem ;) |
4 participants
Problem (Critical + High — C1, H2)
The credit/debit Numscript templates interpolate account names directly into the script body (
@{{ .source }}). But:type:"WALLET"subjects only checkedIdentifier != ""(neither the content norBalancewas validated);balanceNameRegexwas unanchored ([0-9A-Za-z_-]+), soMatchStringaccepted any string containing at least one valid character (e.g.balance:injected,x\n@world).An
identifier/balance/balance-name containing a newline and Numscript tokens could close thesourceblock and inject arbitrary statements executed with the service's ledger credentials (e.g.send [USD *] (source = @world destination = @attacker:main)).Fix
balanceNameRegex→^[0-9A-Za-z_-]+$(a name must be a single clean segment).identifierandbalanceof WALLET subjects viaaccounts.ValidateAddress(the same guarantee the already-safe ACCOUNT branch relies on).walletIDbefore building debit/credit scripts.Possible follow-up defense-in-depth: bind sources as
account $sourceNvariables instead of interpolating them — left out here to avoid destabilizing the exact-Numscript assertions in the existing tests.Tests
TestBalancesCreate: names with an account separator and with whitespace + Numscript tokens → 400.TestWalletsCredit: injection via thebalanceand via theidentifierof a WALLET subject → 400.From the in-depth repository review.